Test for bad path overrides with summaries

Bad path overrides are currently detected to issue warnings in cases where path
overrides are not suitable and have exhibited buggy behavior in the past.
Unfortunately though it looks like some false positives are being issued,
causing unnecessary confusion about `paths` overrides.

This commit fixes the detection of these "bad path overrides" by comparing
`Summary` dependencies (what's written down in `Cargo.toml`) rather than
comparing the `Cargo.toml` of the override with `Cargo.lock`. We're guaranteed
that the package we're overridding has already been resolved into `Cargo.lock`,
so we know that if the two `Cargo.toml` files are equivalent we'll continue
with the same crate graph.

I'm not actually entirely sure why I originally thought it'd be better to go
through the `Cargo.lock` comparison route. Unfortunately that doesn't take into
account optional deps which aren't in `Cargo.lock` but are in `Cargo.toml` of
the override, causing the false positive. This method, however, simply ensures
that the two dependency lists are the same.

Closes #3313

diff --git a/src/cargo/core/dependency.rs b/src/cargo/core/dependency.rs
index 23cb71034b4d73e7491d325a6723626f61672dfb..377f21dee061c2355629aed191e6ecbaa57f1fec 100644 (file)
--- a/src/cargo/core/dependency.rs
+++ b/src/cargo/core/dependency.rs
@@ -11,7 +11,7 @@ use util::{CargoError, CargoResult, Cfg, CfgExpr, ChainError, human, Config};
 
 /// Information about a dependency requested by a Cargo manifest.
 /// Cheap to copy.
-#[derive(PartialEq, Clone ,Debug)]
+#[derive(PartialEq, Clone, Debug)]
 pub struct Dependency {
     inner: Rc<DependencyInner>,
 }

diff --git a/src/cargo/core/registry.rs b/src/cargo/core/registry.rs
index a798a49a5044faba36a0a182f9eaa4d1b1e27305..ec458f8fb9c37a177aead8d7e95e4503cbca1b6c 100644 (file)
--- a/src/cargo/core/registry.rs
+++ b/src/cargo/core/registry.rs
@@ -290,23 +290,7 @@ impl<'cfg> PackageRegistry<'cfg> {
     fn warn_bad_override(&self,
                          override_summary: &Summary,
                          real_summary: &Summary) -> CargoResult<()> {
-        let real = real_summary.package_id();
-        // If we don't have a locked variant then things are going quite wrong
-        // at this point, but we've already emitted a warning, so don't worry
-        // about it.
-        let map = match self.locked.get(real.source_id()) {
-            Some(map) => map,
-            None => return Ok(()),
-        };
-        let list = map.get(real.name()).chain_error(|| {
-            human(format!("failed to find lock name of {}", real))
-        })?;
-        let &(_, ref real_deps) = list.iter().find(|&&(ref id, _)| {
-            real == id
-        }).chain_error(|| {
-            human(format!("failed to find lock version of {}", real))
-        })?;
-        let mut real_deps = real_deps.clone();
+        let mut real_deps = real_summary.dependencies().iter().collect::<Vec<_>>();
 
         let boilerplate = "\
 This is currently allowed but is known to produce buggy behavior with spurious
@@ -322,7 +306,7 @@ http://doc.crates.io/specifying-dependencies.html#overriding-dependencies
 ";
 
         for dep in override_summary.dependencies() {
-            if let Some(i) = real_deps.iter().position(|id| dep.matches_id(id)) {
+            if let Some(i) = real_deps.iter().position(|d| dep == *d) {
                 real_deps.remove(i);
                 continue
             }

diff --git a/tests/overrides.rs b/tests/overrides.rs
index 2ba54fbc271802a90d8b519545ee8a9f55235510..d92f81791ccd187187071e20124d4e38f666b842 100644 (file)
--- a/tests/overrides.rs
+++ b/tests/overrides.rs
@@ -1013,3 +1013,93 @@ fn replace_to_path_dep() {
     assert_that(p.cargo_process("build"),
                 execs().with_status(0));
 }
+
+#[test]
+fn paths_ok_with_optional() {
+    Package::new("bar", "0.1.0").publish();
+
+    let p = project("local")
+        .file("Cargo.toml", r#"
+            [package]
+            name = "local"
+            version = "0.0.1"
+            authors = []
+
+            [dependencies]
+            foo = { path = "foo" }
+        "#)
+        .file("src/lib.rs", "")
+        .file("foo/Cargo.toml", r#"
+            [package]
+            name = "foo"
+            version = "0.1.0"
+            authors = []
+
+            [dependencies]
+            bar = { version = "0.1", optional = true }
+        "#)
+        .file("foo/src/lib.rs", "")
+        .file("foo2/Cargo.toml", r#"
+            [package]
+            name = "foo"
+            version = "0.1.0"
+            authors = []
+
+            [dependencies]
+            bar = { version = "0.1", optional = true }
+        "#)
+        .file("foo2/src/lib.rs", "")
+        .file(".cargo/config", r#"
+            paths = ["foo2"]
+        "#);
+
+    assert_that(p.cargo_process("build"),
+                execs().with_status(0).with_stderr("\
+[COMPILING] foo v0.1.0 ([..]foo2)
+[COMPILING] local v0.0.1 ([..])
+[FINISHED] [..]
+"));
+}
+
+#[test]
+fn paths_add_optional_bad() {
+    Package::new("bar", "0.1.0").publish();
+
+    let p = project("local")
+        .file("Cargo.toml", r#"
+            [package]
+            name = "local"
+            version = "0.0.1"
+            authors = []
+
+            [dependencies]
+            foo = { path = "foo" }
+        "#)
+        .file("src/lib.rs", "")
+        .file("foo/Cargo.toml", r#"
+            [package]
+            name = "foo"
+            version = "0.1.0"
+            authors = []
+        "#)
+        .file("foo/src/lib.rs", "")
+        .file("foo2/Cargo.toml", r#"
+            [package]
+            name = "foo"
+            version = "0.1.0"
+            authors = []
+
+            [dependencies]
+            bar = { version = "0.1", optional = true }
+        "#)
+        .file("foo2/src/lib.rs", "")
+        .file(".cargo/config", r#"
+            paths = ["foo2"]
+        "#);
+
+    assert_that(p.cargo_process("build"),
+                execs().with_status(0).with_stderr_contains("\
+warning: path override for crate `foo` has altered the original list of
+dependencies; the dependency on `bar` was either added or\
+"));
+}